Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Akshaj class settings #62

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AkshajK
Copy link
Contributor

@AkshajK AkshajK commented Jan 3, 2021

Allow for editing of: class description, term, contact email

Displays these three features on the course contents page for everyone to see (if they are put in by instructor)

Copy link
Member

@JumanaFM JumanaFM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Akshaj for your PR, the additions you made look good. However, I wonder why you have removed "institution" from the model? It is something that we actually want to store. Can you please keep it in the model and add it to UI (don't make it mandatory).

@AkshajK
Copy link
Contributor Author

AkshajK commented Jan 6, 2021

regarding the institution, I noticed that there already was an institution field in the codebase, which described the institution ID (and linked to a institution object). I had added another institution fieldd which was just a string earlier (but commented out), so I removed it. The already existing institution field is an object (which makes sense, we don't want to store MIT and Massachusetts Institute of Technology as seperate institutions i guess) but then it makes changing it more complicated (we'd need a dropdown as well as a "add new option" so I thought for now I wouldn't put it in the class settings tab

However, i put it in the frontend so if the institution field is ever populated, it will show up in the class dashboard!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants